Skip to content

Conversation

@CTTY
Copy link
Contributor

@CTTY CTTY commented Aug 15, 2025

Which issue does this PR close?

What changes are included in this PR?

  • Implemented RewriteFilesAction
  • Added SnapshotValidator and implemented it for RewriteFilesOperation
  • Added logic to process delete files in SnapshotProducer and generate manifests

Are these changes tested?

Not tested so far


for entry in manifest.entries() {
match entry.content_type() {
DataContentType::Data => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the exact same code blocks? So why does the ManifestEntry's DataContentType matter? Why not just do:

for entry in manifest.entries() {
    if snapshot_producer
        .deleted_data_files
        .iter()
        .any(|f| f.file_path == entry.data_file.file_path())
     {
         delete_entries.push(copy_with_deleted_status())
     }
        
}

Copy link
Contributor Author

@CTTY CTTY Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks very similar with one little difference: one block iterates thru deleted_data_files and another iterates thru deleted_delete_files

@CTTY CTTY force-pushed the ctty/pure-rewrite branch from 71a696f to 336c974 Compare November 5, 2025 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add RewriteFiles support to iceberg-rust

2 participants